-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Allow SDL GPU to opt into additional Vulkan features #14204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This adds SDL_HINT_VULKAN_REQUEST_API_VERSION to allow requesting a specific API version when SDL creates the Vulkan instance.
The patch version shouldn't be relevant to the API and a new major version will likely require an entirely new renderer anyway. The benefit of this approach is that it massively simplifies parsing of the hint.
We hardcode to 1.0 to provide maximum driver compatibility. I'm totally fine with allowing an opt-in to higher versions. |
include/SDL3/SDL_hints.h
Outdated
* This hint should be set before creating a Vulkan window. Expects a positive | ||
* integer. E.g. 3 for Vulkan 1.3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable to allow specifying the full version and will be more intuitive for users. This is easily implemented with, e.g.
if (SDL_sscanf(hint, "%d.%d", &major, &minor) == 2) {
...
} else {
// Parse error...
}
Co-authored-by: Sam Lantinga <[email protected]>
This reverts commit 6c4d2c0.
This needs a bit more time in the oven, I think. |
Introduces a new property to use when creating a Vulkan renderer. SDL_PROP_GPU_DEVICE_CREATE_VULKAN_ADDITIONAL_FEATURES_POINTER
Since the developer can now pass additional features to be activated, this name makes more obvious that these get enabled by SDL regardless.
I added a workflow to opt-into Vulkan device features using properties. I made sure the default behavior is unaffected. The usage code in my application look like this: props := sdl.CreateProperties()
sdl.SetBooleanProperty(props, sdl.PROP_GPU_DEVICE_CREATE_SHADERS_SPIRV_BOOLEAN, true)
sdl.SetBooleanProperty(props, sdl.PROP_GPU_DEVICE_CREATE_DEBUGMODE_BOOLEAN, true)
sdl.SetStringProperty(props, sdl.PROP_GPU_DEVICE_CREATE_NAME_STRING, "vulkan")
// Enable Vulkan 1.1 feature
vulkan_11_features := vk.PhysicalDeviceVulkan11Features {
sType = vk.StructureType.PHYSICAL_DEVICE_VULKAN_1_1_FEATURES,
shaderDrawParameters = true,
}
sdl.SetPointerProperty(props, sdl.PROP_GPU_DEVICE_CREATE_VULKAN_ADDITIONAL_FEATURES_POINTER, rawptr(&vulkan_11_features))
result.gpu = sdl.CreateGPUDeviceWithProperties(props) EDIT: This still needs to handle errors from unsupported features. |
KHR_portability_subset structure was being overwritten by opt-in features.
This seems like a reasonable workflow to me. @thatcosmonaut, thoughts? |
Yeah that does seem reasonable. Is this stable if we have to add fields to the struct? |
If increasing the Vulkan version often means chaining additional device creation structures, maybe the Vulkan version should also be a property instead of a hint? |
I'm not sure I understand.
I was thinking about the same thing. Right now, I'm looking into error handling. As it stands, the vkCreateDevice() call crashes when unsupported features are requested. |
We guarantee ABI stability in SDL, so if we add something we can't make future code changes that will cause an executable to break if a client updates the SDL3 DLL for example. |
Yes, this change doesn't affect any public structures, so this is ABI safe. |
Oh I just realized it's literally a pointer to the Vulkan struct, that makes sense. |
My current solution looks like this: // SDL_gpu.h
#define SDL_PROP_GPU_DEVICE_CREATE_VULKAN_OPTIONS_POINTER "SDL.gpu.device.create.vulkan.options"
/**
* A structure specifying additional options when using Vulkan.
*
* When no such structure is provided, SDL will use Vulkan API version 1.0 and a minimal set of features.
*
* \since This struct is available since SDL 3.4.0.
*
*/
typedef struct SDL_GPUVulkanOptions
{
Uint32 vulkan_api_version; /**< The Vulkan API version to request for the instance. */
void *physical_device_features_1_0; /**< Pointer to a VkPhysicalDeviceFeatures struct. */
void *physical_device_features_1_1; /**< Pointer to a VkPhysicalDeviceVulkan11Features struct. */
void *physical_device_features_1_2; /**< Pointer to a VkPhysicalDeviceVulkan12Features struct. */
void *physical_device_features_1_3; /**< Pointer to a VkPhysicalDeviceVulkan13Features struct. */
} SDL_GPUVulkanOptions; Providing the various structs separately rather than in a chain makes validation and error-reporting much easier. Does this look like a workable solution? |
I would prefer your original implementation which just added a single chained structure to the options. It's more flexible, and once you're using properties, you're generally speaking outside the normal development path and responsible for doing so safely. |
Yeah, I started reworking it almost immediately after I posted that yesterday. It occurred to me that this structure wasn't very forward compatible. |
Also replaces the previously created hint
I just pushed the first version of the new workflow. // SDL_gpu.h
#define SDL_PROP_GPU_DEVICE_CREATE_VULKAN_OPTIONS_POINTER "SDL.gpu.device.create.vulkan.options"
/**
* A structure specifying additional options when using Vulkan.
*
* When no such structure is provided, SDL will use Vulkan API version 1.0 and a minimal set of features.
* The feature list is only allowed to contain the following structures:
* - VkPhysicalDeviceFeatures2
* - VkPhysicalDeviceVulkan11Features
* - VkPhysicalDeviceVulkan12Features
* - VkPhysicalDeviceVulkan13Features
*
* \since This struct is available since SDL 3.4.0.
*
*/
typedef struct SDL_GPUVulkanOptions
{
Uint32 vulkan_api_version; /**< The Vulkan API version to request for the instance. Use Vulkan's VK_MAKE_VERSION or VK_MAKE_API_VERSION. */
void *feature_list; /**< Pointer to the first element of a list of Vulkan physical device feature structs. */
} SDL_GPUVulkanOptions; The usage code now looks something like this: props := sdl.CreateProperties()
sdl.SetBooleanProperty(props, sdl.PROP_GPU_DEVICE_CREATE_SHADERS_SPIRV_BOOLEAN, true)
sdl.SetBooleanProperty(props, sdl.PROP_GPU_DEVICE_CREATE_DEBUGMODE_BOOLEAN, true)
sdl.SetStringProperty(props, sdl.PROP_GPU_DEVICE_CREATE_NAME_STRING, "vulkan")
// Enable additional Vulkan features if needed
vulkan_10_features := vk.PhysicalDeviceFeatures2 {
sType = vk.StructureType.PHYSICAL_DEVICE_FEATURES_2,
features = {
wideLines = true,
},
}
vulkan_11_features := vk.PhysicalDeviceVulkan11Features {
sType = vk.StructureType.PHYSICAL_DEVICE_VULKAN_1_1_FEATURES,
shaderDrawParameters = true,
}
vulkan_12_features := vk.PhysicalDeviceVulkan12Features {
sType = vk.StructureType.PHYSICAL_DEVICE_VULKAN_1_2_FEATURES,
scalarBlockLayout = true,
}
vulkan_13_features := vk.PhysicalDeviceVulkan13Features {
sType = vk.StructureType.PHYSICAL_DEVICE_VULKAN_1_3_FEATURES,
inlineUniformBlock = true,
}
// Create list
vulkan_10_features.pNext = rawptr(&vulkan_11_features)
vulkan_11_features.pNext = rawptr(&vulkan_12_features)
vulkan_12_features.pNext = rawptr(&vulkan_13_features)
// Set SDL property
vulkan_options := sdl.GPUVulkanOptions {
vulkan_api_version = vk.API_VERSION_1_3,
feature_list = rawptr(&vulkan_10_features),
}
sdl.SetPointerProperty(props, sdl.PROP_GPU_DEVICE_CREATE_VULKAN_OPTIONS_POINTER, rawptr(&vulkan_options))
result.gpu = sdl.CreateGPUDeviceWithProperties(props) The requested features are now error-checked early, too. It prints out the individual unsupported features which were requested:
|
I should add.
Everything else causes SDL to fail to initialize. I did this, because some oversights were made in Vulkan 1.0. There are struct types which don't contain their type and a pointer to a next structure. But that information is required for SDL to iterate over the list. |
It's worth noting that VkPhysicalDeviceVulkan11Features was actually added in Vulkan 1.2 (!) and the only way to enable Vulkan 1.1 features with a Vulkan 1.1 instance is to chain feature-specific structs like VkPhysicalDeviceShaderDrawParameterFeatures. |
In that case, I think it would be better to remove the whitelist and allow passing arbitrary struct chains into device creation. As @slouken said, when you're using properties, you're responsible for using them correctly. |
Agreed. |
Looking a bit deeper into the specs, it looks like Vulkan 1.1 is an annoying special case. At that point, they still haven't quite figured out how they want to handle future version extensions yet. My current implementation works with 1.2+. Saying that using props means you're responsible for using them correctly is fine, but I'd still like SDL to do some sanity checking. At least enough to where it can fail gracefully and report useful error messages rather than just crashing. |
Next draft. (Still WIP.) /**
* A structure specifying additional options when using Vulkan.
*
* When no such structure is provided, SDL will use Vulkan API version 1.0 and a minimal set of features.
* The feature list gets passed to the vkCreateInstance function and allows requesting additional
* features.
*
* \since This struct is available since SDL 3.4.0.
*
*/
typedef struct SDL_GPUVulkanOptions
{
Uint32 vulkan_api_version; /**< The Vulkan API version to request for the instance. Use Vulkan's VK_MAKE_VERSION or VK_MAKE_API_VERSION. */
void *feature_list; /**< Pointer to the first element of a list of structs to be passed to device creation. */
void *vulkan_10_physical_device_features; /**< Pointer to a VkPhysicalDeviceFeatures struct to enable additional Vulkan 1.0 features. */
} SDL_GPUVulkanOptions;
|
E.g. Vulkan 1.3 features will be ignored if API version 1.2 or lower is requested.
This may have been answered already so apologies in advance, but is the slang compiler capable of using |
I am working on something like that. This pull request started off as a version selector, but morphed into something more flexible. If you're interested in specific features, please post them here. I'll see if they can be made to fit. @flibitijibibo |
Currently
SDL_gpu_vulkan.c
hardcodes API version 1.0 when creating the Vulkan instance for the program. This creates a problem for my use-case.I'd like to use Slang for shader development, but SPIR-V 1.0 support is experimental. I tried two solutions and both work:
I looked at the Vulkan documentation (specifically this and this). It says "Deprecated items will still work in current Vulkan implementations", which makes me prefer option 2. A Vulkan application requesting 1.3, but using only 1.0 features should work just fine. And using a hint doesn't change SDL's default behavior.
Since I don't know why this was hardcoded, I figured I'd first ask if there's any chance of this merge request going anywhere. Before I waste time cleaning something up that's not going to be merged for reasons I didn't know of.